Skip to content

release: v0.1.0 - resiliency middleware, pre-v1 API, performance and test quality - #4

Merged
oswaldom-code merged 59 commits into
mainfrom
develop
Jul 25, 2026
Merged

release: v0.1.0 - resiliency middleware, pre-v1 API, performance and test quality#4
oswaldom-code merged 59 commits into
mainfrom
develop

Conversation

@oswaldom-code

@oswaldom-code oswaldom-code commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Release v0.1.0: all five phases of the development plan are complete.

Summary

  • Fase 1 — correctness blockers: streaming body readable after builder SetTimeout, Timeout(d<=0) as no-op, invalid TokenBucket config as unlimited no-op, IsFailure evaluated outside the breaker mutex, and circuit breaker results gated by admission generation.
  • Fase 2 — pre-v1 API: New returns *Client (struct, no interface), RateLimiter reduced to TryAcquire + WaitContext, RoundTripperFunc exported (internal package removed), BackoffFunc now receives the previous response with a WithRetryAfter decorator, PerHostRateLimiter removed, opinionated DecodeJSON added, CircuitState.String.
  • Fase 3 — debt and performance: request body closed on middleware short-circuits, exponential backoff saturates on int64 overflow, nil ctx defaults in Do, wait timers stopped on cancellation, byte-backed builder bodies survive re-execution, retry skips the clone on attempt 0 (16 to 13 allocs full-stack), token wait precomputed, lazy builder maps.
  • Fase 4 — test quality: weak assertions strengthened, middleware interaction semantics pinned, streaming >10 MB covered, State/Tokens covered with deterministic half-open observation, remaining gaps closed. Coverage 95.2% (root package).
  • Fase 5 — release: benchmarks re-measured post-optimization and published in README with honesty caveats, Makefile coverage/ci targets repaired, CHANGELOG 0.1.0 cut.

Benchmarks (2026-07-25)

Full middleware stack: ~1 us, 13 allocs/op. As a wrapper (equivalent config), 910 ns/12 allocs vs net/http floor at 1.92x. See the README Benchmarks section and benchmarks/REPORT.md for tables and caveats.

Verification

  • make test-race green throughout (every task committed with the race suite passing)
  • make lint exit 0
  • Coverage 95.2% on the root package

Oswaldo Montaño and others added 30 commits January 19, 2026 14:16
Add initial project setup with README, Makefile, and configuration files
- Add `cache-dependency-path: go.mod` to all `setup-go` steps in the CI workflow to ensure proper caching.
- Fix a typo in `.golangci.yml` for the gocritic `commentedOutCode` disabled check.
- Remove `go.sum` from the `git diff --exit-code` command in the `.github/workflows/ci.yml` build job to only verify `go.mod`.
- Comment out the entire `.github/workflows/ci.yml` file to temporarily disable the GitHub Actions CI pipeline.
Fix: CI cache dependency paths and correct golangci configuration typo
Core HTTP client with middleware chain and resiliency patterns:
client, options, transport, middleware chain, timeout, retry with
backoff strategies, circuit breaker, rate limiting, logging, metrics,
error classification, fluent RequestBuilder and object pooling.

Includes unit tests, examples and benchmarks. Zero dependencies.
- timeout: cancel the timeout context on Body.Close, not on RoundTrip
  return, so streaming/chunked bodies aren't aborted mid-read
- retry: skip draining/closing the body on the final attempt, since
  that response is returned to the caller

Adds httptest.Server regression tests that read the body after
RoundTrip returns.
Split retryRoundTripper.RoundTrip into canRetry, prepareRetry and
drainAndClose, lowering cognitive complexity below the linter
threshold and removing the //nolint:gocognit directive.
Add MaxHalfOpenRequests (default 1) and SuccessThreshold (default 1)
so Half-Open admits a bounded number of concurrent probes and closes
only after enough consecutive successes. Previously the mutex was
released between allowRequest and recordResult, letting every
concurrent request through in Half-Open.

Also split recordResult into recordClosedResult/recordHalfOpenResult,
order callees before callers, and add concurrency regression tests.
Switch backoff jitter from math/rand/v2 (Go 1.22+) to math/rand so the
stdversion warnings go away while go.mod stays at 1.21. Also adopt the
min builtin in place of manual max-capping.
Move the package from the httpclient/ subdirectory to the module root and
rename it from httpclient to rhttp, dropping the go-httpclient/httpclient
import stutter. Updates go.mod, every import and package clause, the
sentinel error prefixes (rhttp:), README, Makefile, .golangci.yml, CLAUDE.md
and the example.

No behavior change: build, race tests and lint pass under the new path.

BREAKING CHANGE: import path is now github.com/oswaldom-code/rhttp and the
package identifier is rhttp (was .../go-httpclient/httpclient, httpclient).
Uncomment .github/workflows/ci.yml so the test, lint, build and benchmark
jobs run on pull requests and pushes to main. The Go 1.21 matrix job now
passes since backoff no longer imports math/rand/v2.
RoundTrip mutated req.Body of the caller's request via prepareRetry,
violating the http.RoundTripper contract (RoundTrip must not modify the
request) and leaking per-attempt changes between attempts. Split into
prepareRequest (clones the request per attempt, rewinds the body via
GetBody only on retries) and waitBackoff. Adds regression test
TestRetry_DoesNotMutateOriginalRequest.
The middleware stored next on a shared struct created outside the
closure, so applying one CircuitBreaker value to two chains made them
share state and let the second application overwrite the first chain's
transport. Move breaker creation into the closure (independent instance
per application) and hold next in a per-chain wrapper. Add
NewCircuitBreaker for the opposite, intentional case: a breaker whose
state is shared across chains via Middleware().
Replace the fragile strings.Contains fallback in classifyError with
structured checks: syscall.ECONNREFUSED/ECONNRESET/EHOSTUNREACH/
ENETUNREACH for connection errors and x509 error types for TLS. Split
into classifyTLS and classifyConnection helpers. Remove ErrKindTemporary,
which no branch produced and whose retryable semantics IsRetryable
already covers. Rewrite the tests that relied on plain-string errors to
use real typed errors, and add TestClassify_AllKindsAreReachable to fail
if any ErrorKind becomes orphaned.
DefaultIsRetryable retried on any non-nil error, so TLS verification
failures and caller cancellations burned every attempt and hid the real
error. Route error decisions through Classify(err).Kind.IsRetryable() so
only transient kinds (timeout, connection, DNS) retry; TLS and canceled
do not. Add TestRetry_RespectsErrorClassification table and migrate the
existing retry tests off plain-string errors onto typed ones.
DefaultIsFailure treated any non-nil error as an upstream failure, so a
burst of caller cancellations (frontend navigation, shutdown) could open
the circuit against a healthy upstream. Classify the error and skip
ErrKindCanceled; timeouts still count as failures since a slow upstream
is a degraded one. Add TestCircuitBreaker_ClientCancellationsDoNotOpenCircuit
and TestCircuitBreaker_TimeoutsOpenCircuit.
drainAndClose read the entire error body before each retry so a large
5xx payload added latency and bandwidth at the worst moment. Cap the
drain with io.LimitReader(maxDrainBytes); bodies past the limit leave the
connection to be closed instead of reused. Add TestRetry_DrainIsBounded.
Metrics emitted req.URL.Path raw. When a recorder exports Path as a
metrics label, per-ID REST paths create one time series per request and
grow Prometheus memory without bound.

Path is now empty unless a PathNormalizer is provided, deferring the
cardinality decision to the caller. Provide func(p string) string that
collapses high-cardinality segments to a template.

Diagnostic 4.3.
…mbers

The 35% faster claim compared wrapper overhead over a no-op transport,
not real network requests, and read as a network-performance claim the
benchmarks do not support.

Rename BenchmarkClient_* to BenchmarkMiddlewareOverhead_* to reflect
what they measure, rewrite the Benchmarks section with an explicit
methodology note, and run make bench with -count=5 for reproducibility.

Diagnostic 2.1.
The 100% test coverage - 101 tests line is a claim that expires on
every PR. Defer to the dynamic Codecov badge as the single source of
truth for coverage.

Diagnostic 2.2.
The chain applies the first middleware as the outermost wrapper, and
Timeout placement relative to Retry silently selects total-budget vs
per-attempt timeout semantics - the kind of undocumented detail that
causes incidents.

Add a Middleware Order section covering the outermost-first rule, both
timeout patterns, and the Retry/CircuitBreaker interaction, plus
runnable ExampleRetry_totalBudget and ExampleRetry_perAttemptTimeout.

Diagnostic 4.4.
Align the recommended middleware order with the diagnostic: Retry now
sits outside CircuitBreaker (... RateLimit -> Retry -> CircuitBreaker)
so every attempt consults the circuit and a tripped breaker
short-circuits the remaining attempts.

Updated doc.go package godoc, CLAUDE.md, and the README order line,
example block, and interaction table.
New exported symbols shipped without godoc and rendered bare on
pkg.go.dev. Add doc comments to SharedCircuitBreaker, NewCircuitBreaker,
Middleware, State and the MetricsConfig.PathNormalizer field.

Document SetBody buffering: bodies up to 10 MB are buffered so retries
can rewind them; larger bodies stream and are sent once without retry.
Without this note the >10 MB no-retry case is a silent surprise.
The Motivation and Usage Modes sections were the last Spanish prose in
an otherwise English README. Unify the language for general adoption.

Diagnostic 5.2 (T3).
Oswaldo Montaño added 25 commits July 25, 2026 11:32
Timeout(0) wrapped every request in an already-expired context, so all
requests failed with deadline exceeded (and a negative duration did the
same). Follow the project convention that an invalid config returns the
next RoundTripper unchanged.
A non-positive rate produced a 1/rate wait time (division by zero or
negative), busy-looping WaitContext at 100% CPU (forever via the deprecated
Wait()). A burst below 1 left maxTokens at 0 so TryAcquire never succeeded,
blocking forever. Validate in the constructor and return an unlimited bucket
that allows every request, matching the invalid-config-is-a-no-op convention
used by Timeout and RateLimit.
recordResult held cb.mu while calling the user-supplied IsFailure callback.
A callback that inspected the breaker (e.g. State(), which locks cb.mu)
deadlocked, since sync.Mutex is not reentrant. Evaluate IsFailure before
acquiring the lock and pass the result into the state machine.
A request admitted in one state could record its result after the breaker
had transitioned, corrupting the current episode: a slow Closed request
completing during Half-Open ran recordHalfOpenResult, closing the circuit
and freeing the real probe's budget. Track a generation that bumps on every
state transition; allowRequest returns the admission generation and
recordResult discards results whose generation no longer matches. This also
makes the Open case in recordResult genuinely unreachable (C13), with an
accurate comment.
New now returns *Client, R() is a method on it, and the type-assert
fallback in the old free R() is gone. Pre-v1 breaking change per plan A1.
The interface is now TryAcquire plus WaitContext, so an adapter over an
x/time/rate style limiter fits in two one-line methods. The deprecated
TokenBucket.Wait concrete method is deleted pre-v1 per plan A2.
The adapter now lives in middleware.go with a godoc example showing a
five-line custom middleware. All tests migrate off internal.RoundTripperFunc,
which leaves the internal package empty, so it is deleted. Plan A4.
bench_test.go moves to the exported *rhttp.Client after A1. REPORT.md is the
2026-07-25 re-run: allocations unchanged (15 overhead, 79 E2E), rhttp still
the cheapest wrapper at 1.00x overhead.
The type did not satisfy RateLimiter and could not plug into the RateLimit
middleware, so it was an exported dead end. Deleted per plan A5; it can
return post-v1 together with a dedicated per-host middleware.
BackoffFunc gains a resp parameter carrying the response of the attempt
that triggered the retry (nil when it produced none). The seven strategies
ignore it and keep their pure attempt-to-duration logic at zero allocs
(verified against a 5-sample baseline). RetryConfig.Backoff is now typed
as BackoffFunc and the retry loop threads the previous response through.

New WithRetryAfter decorator waits max(base, server hint) on 429/503,
parsing both delay-seconds and HTTP-date formats. Plan A3.
DecodeJSON always drains and closes the body, treats status >= 300 as an
error without decoding, and streams the decode on 2xx. CircuitState gains
a lowercase String method. Plan A6+A7; RetryConfig.Backoff typing already
landed with A3 and the builder one-shot godoc belongs to C10.
A RoundTripper owns the request body even when it fails the request.
Circuit-open, rate-limited, canceled Retry-After waits, and canceled or
unpreparable retry attempts now release it via closeRequestBody. Plan C6.
base shifted by attempts around 37 wrapped negative, producing negative
waits that time.After treats as zero and disabling backoff entirely. The
three exponential variants now saturate at maxDuration once the product
no longer fits in int64. Plan C7.
Do(nil, req) panicked in req.Clone; a nil ctx now falls back to
context.Background. The three select waits (retry backoff, token refill,
Retry-After period) switch from time.After to a stopped time.Timer so a
canceled context releases the timer immediately. Plan C8+C9.
resolveBody handed back the same bytes.Reader on every execute, so a
second run sent an empty body with a mismatched ContentLength; it now
builds a fresh reader from bodyBytes. The header copy loop becomes a
direct assignment (Do clones the request, sharing is safe), and the
builder's one-shot, single-goroutine contract is documented. Plan C10+P3.
Do already clones the request before it enters the chain, so cloning
again for attempt 0 only protected retries, which build their own clone.
MiddlewareOverhead_WithRetry drops 7 to 4 allocs/op (456 to 250 ns/op)
and AllMiddleware 16 to 13 allocs/op. Plan P1.
TokenBucket computes its per-token refill wait once in the constructor
instead of under the mutex on every WaitContext iteration. RequestBuilder
allocates the query and path parameter maps on first use, and the
DecorrelatedJitterBackoff godoc now states that concurrent sequences
sharing one instance correlate their delays. Plan C11+P4.
The two no-retry guards now fail with syscall.ECONNREFUSED, a genuinely
retryable error, so they prove the guard and not the error class. Basic
auth asserts the exact encoded header, the builder timeout uses
errors.Is with context.DeadlineExceeded, and the failure-count reset
test gains its control case: three straight failures do open. Plan Q1.
Timeout outside Retry enforces a total budget and cuts the run mid
backoff; Retry outside Timeout gives every attempt a fresh deadline; and
a breaker tripped mid-retry short-circuits the remaining budget because
ErrCircuitOpen is not retryable. Plan Q2.
A body one byte over the 10 MB cap must stream: GetBody stays nil, the
transport receives every byte exactly once, and a retryable 503 does not
trigger a second attempt because the reader cannot be replayed. Plan Q3.
SharedCircuitBreaker.State is asserted through a full closed-open-half-
open-closed cycle, observing half-open deterministically with a probe
held in flight. TokenBucket.Tokens gets its first coverage. The reset
timeout sleeps move from 5ms of slack to at least 50ms to survive loaded
CI runners. Plan Q4.
Rate limiter: WaitContext on a pre-canceled ctx, HTTP-date Retry-After,
ctx cutting a Retry-After wait short, and 50 goroutines racing the
RespectRetryAfter bookkeeping. Builder: malformed URL, JSON and XML
marshal error branches, XML happy path, custom Execute method, and path
parameter escaping. Plan Q5.
coverage-summary now regenerates the profile instead of reading whatever
coverage.out was left on disk (a stale one from the old module path made
it fail), the ci target gets the deps rule it referenced but never had,
and .PHONY covers every declared target.
The README Benchmarks section now carries the 2026-07-25 post-
optimization numbers: 13 allocs for the full stack, 910ns/12 allocs as a
wrapper against equivalently configured competitors (net/http floor at
1.92x), the loopback E2E table, and the honesty caveats that qualify
both. REPORT.md is the regenerated canonical snapshot. Plan R1.
@oswaldom-code oswaldom-code changed the title release: rename to rhttp, Phase 1 fixes and diagnostic remediation release: v0.1.0 - resiliency middleware, pre-v1 API, performance and test quality Jul 25, 2026
@oswaldom-code oswaldom-code reopened this Jul 25, 2026
Main carries the squash of PR #3, whose tree is identical to develop at
2cce947. Develop is a strict superset, so this merge keeps the develop
tree unchanged and only records the ancestry so the release PR applies
cleanly.
@oswaldom-code
oswaldom-code merged commit 063b7d6 into main Jul 25, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant